feat(sql): generate-vs-hand-maintain follow-up for the function catalog (#225) - #233
Merged
Merged
Conversation
…og (#225) Rebased onto main now that #227 (the catalog crate + generalized ClickHouse-builtin mechanism) and #226 (the QueryExpr rename) have landed there — this PR's own content is unchanged, just reapplied cleanly on top of current main instead of the now-deleted feat/sql-function-catalog-225 branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
force-pushed
the
sql-function-catalog-generation
branch
from
August 22, 2026 20:56
564462d to
e989bd7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of #225 -- completes item 3 of its "Proposed direction" ("Where
feasible, generate rather than hand-maintain"), which #227 deliberately left
undone. Stacked on top of #227 (
feat/sql-function-catalog-225): this PR'sbase is that branch, not
main, sinceasap-sql-function-catalogdoesn'texist on
mainyet.Issue #225 names two introspectable sources and treats them differently:
in-process, no external dependency, no infra decision needed.
system.functions-- needs a live/embedded ClickHouse,which this repo has no CI infra for.
1. DataFusion registry drift detection (real, CI-exercised Rust)
crates/frontend-sql/src/sql/mod.rsgets two new tests (mod catalog_drift)that build a real
SessionContextthe same waySqlLowerer::build_context()does and:
asap_sql_function_catalog::NATIVE_FUNCTIONS, is one of the catalog's ownCLICKHOUSE_BUILTINSstub registrations, or is on a new, documentedKNOWN_UNMAPPED_NATIVE_FUNCTIONSallow-list (each entry has a reason:no
AggIntentshape yet, or explicitly rejected elsewhere);KNOWN_UNMAPPED_NATIVE_FUNCTIONSentry is still a realDataFusion name (so the allow-list itself can't quietly rot).
This turns a future DataFusion version bump that adds/renames/removes a
builtin aggregate into a
cargo testfailure instead of silent catalogdrift -- no new CI infra, it's part of the existing
cargo test --workspace.I verified the test actually catches drift by temporarily removing an entry
from the allow-list and confirming it fails with a clear message, then
restored it.
Along the way this surfaced two small, pre-existing gaps that the old
hand-written
matchnever had a way to reveal: DataFusion resolvesvar_sample/var_population(its own alias spellings ofvar_samp/var_pop) butNATIVE_FUNCTIONSdoesn't list those spellings. Rather thansilently "fixing" that in this PR, they're recorded on
KNOWN_UNMAPPED_NATIVE_FUNCTIONSwith a comment -- accepting a new spellingis a maintainer's call, and surfacing it rather than deciding it is the
point of this tooling.
2. ClickHouse builtin discovery (dev-only, not CI-wired)
tools/clickhouse/extract_functions.pydiffs ClickHouse's ownsystem.functions(aggregate functions only) againstCLICKHOUSE_BUILTINSand reports:countifthat are reallya base function (
count) plus one of ClickHouse's aggregate combinators(
-If,-Distinct,-Array, ...), which ClickHouse doesn't enumerate asits own
system.functionsrow -- so a naive diff would misreport these asstale every run; they're called out separately instead.
It uses
chdb(ClickHouse embeddedin-process) rather than a live server --
pip install chdbis the onlysetup step, no Docker, no network service.
tools/clickhouse/README.mdcovers how to run it, and explicitly documents why it's not wired into
CI (no ClickHouse service anywhere in this repo's CI, and that's a
deliberate decision, not an oversight).
It never touches
crates/sql-function-catalog/src/lib.rs-- it's areporting tool for a human to act on, matching the issue's own scope
(existence + arity only;
RewriteKind/semantic is a human judgment call).Explicitly out of scope (per the issue and this follow-up's own scope)
RewriteKind/AggSemanticassignments.ElasticSQLuntouched (separate, bigger gap per SQL: extract dialect builtin-function catalog into independent, generatable data (inspired by polyglot-sql-function-catalogs) #225).Test plan
cargo build --workspace --all-targetscargo test --workspace(all green, including the two newcatalog_drifttests)cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscatalog entry is removed, confirming it actually detects drift
tools/clickhouse/extract_functions.pyrun end-to-end against realembedded ClickHouse via
chdb(installed successfully in thissandbox) -- produced a correct, sensible diff report, including
correctly classifying
countifas combinator-derived rather thanfalsely "stale"
python3 -m unittest tools/clickhouse/test_extract_functions.py(16 tests, all passing) -- covers the catalog-parsing and diff logic
without needing ClickHouse at all
🤖 Generated with Claude Code